-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Selectors API: Make duotone selectors fallback and be scoped #49423
Conversation
Size Change: +623 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
a0fde9e
to
520ea49
Compare
return _wp_array_get( $block_type->selectors, array( 'filter', 'duotone' ), $fallback_selector ); | ||
} | ||
|
||
// Selectors API, not available, check for old experimental selector. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth saying that this is left in for backwards compatibility, and should be removed one day?
520ea49
to
4f4fc3c
Compare
lib/class-wp-duotone-gutenberg.php
Outdated
@@ -349,7 +349,26 @@ public static function render_duotone_support( $block_content, $block ) { | |||
$filter_id = gutenberg_get_duotone_filter_id( array( 'slug' => $slug ) ); | |||
|
|||
// Build the CSS selectors to which the filter will be applied. | |||
$selector = WP_Theme_JSON_Gutenberg::scope_selector( '.' . $filter_id, $duotone_selector ); | |||
$scopes = explode( ',', '.' . $filter_id ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I hadn't to paste this here, but this needs to work differently than the WP_Theme_JSON_Gutenberg::scope_selector
(do not add an extra space).
Ideas for a follow-up PR:
-
The usage of this internal utility has expanded to: settings, duotone, WP_Theme_JSON class. It's worth creating a utility function somewhere. Where?
wp_scope_css_selector( $scope, $selector )
as a name? -
The consumer should be responsible for adding or not adding spaces, not the function itself:
wp_scope_css_selector( $scope . ' ', $selector )
if the consumer wants spaces.wp_scope_css_selector( $scope . ' ', $selector )
if the consumer doesn't want spaces.
Thoughts?
Flaky tests detected in 7e7bf1c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4567826409
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working as expected.
Before: .wp-duotone .wp-block-name After: .wp-duotone.wp-blockname
cc6e325
to
64a505a
Compare
* | ||
* @return {string} Scoped selector. | ||
*/ | ||
function scopeSelector( scope, selector ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it probably makes more sense to keep this function but just modify it.
lib/block-supports/duotone.php
Outdated
@@ -450,3 +471,4 @@ function gutenberg_render_duotone_support( $block_content, $block ) { | |||
add_action( 'wp_enqueue_scripts', array( 'WP_Duotone_Gutenberg', 'output_global_styles' ), 11 ); | |||
add_action( 'wp_footer', array( 'WP_Duotone_Gutenberg', 'output_footer_assets' ), 10 ); | |||
add_filter( 'block_editor_settings_all', array( 'WP_Duotone_Gutenberg', 'add_editor_settings' ), 10 ); | |||
add_filter( 'block_type_metadata_settings', 'gutenberg_migrate_experimental_duotone_support_flag', 10, 2 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a useful technique for migrating more settings over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only problem is that there's no longer any way of knowing if you have experimental support or not. It's causing me problems because the selector that we choose is different depending on whether or not it's experimental.
// May be true even if `filter.duotone` isn't set.
const duotoneSupport = getBlockSupport(
blockType,
'filter.duotone',
false
);
// May be set even if `filter.duotone` is also set.
const experimentalDuotone = getBlockSupport(
blockType,
'color.__experimentalDuotone',
false
);
// Will always produce a selector even if `filter` or `filter.duotone` isn't set.
const duotoneSelector = getBlockCSSSelector(
blockType,
'filter.duotone',
{ fallback: true }
);
EDIT: We could have color.__experimentalDuotone
override filter.duotone
, but that seems wrong to me. If they're both set, I would expect filter.duotone
to override color.__experimentalDuotone
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow.
In the filter, we were only setting filter.duotone
if it hasn't already been set and the color.experimentalDuotone
has been.
A block author shouldn't be using both properties in their raw block.json so if we have color.experimentalDuotone
set at all, then it should be a case from when duotone was experimental.
- In your first example, the check for
filter.duotone
tells us if there is support. - The second example if the default is removed or changed to
null
would tell us if the duotone support was the experimental version. - The third snippet with fallback true would always produce a selector regardless of any support check. We would guard such a call with a check that the
color.experimentalDuotone
support flag wasn't set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that by leaving the __experimentalDuotone
property we could determine when to use that as the selector. I can see from this discussion where I missed adding the guard check to the duotone.js HoC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be working as expected to me, and the code looks good, but I'd rather wait on @ajlende to merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronrobertshaw The changes I made ended up being kind of big again, so I didn't want to merge right away without someone else taking a look. I have nothing else to add, so if things are working well for you, go ahead and merge it.
// Backwards compatibility for color.__experimentalDuotone. This will | ||
// have priority over filter.duotone support. Unfortunately we can't | ||
// prefer filter.duotone because it gets set when __experimentalDuotone | ||
// is set via a block_type_metadata_settings hook. It shouldn't be too | ||
// much of a problem because I would expect consumers to not use both | ||
// at the same time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to do this to fix the experimental selectors in the editor being applied to the root of the block instead of the selector set in __experimentalSelector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we're on the same page here. The supports.filter.duotone
property will only get set if it hasn't been set already and we have supports.color.__experimentalDuotone
set.
So supports.filter.duotone
will tell us if we have duotone support one way or another. Checking if we have the supports.color.__experimentalDuotone
flag set should tell us if we need to handle an experimental duotone selector or use the selectors API requesting selectors.filter.duotone
I've updated the comments added around backwards compatibility of We can also return earlier if there is no duotone support so I relocated those guard conditions as well in 3346b9a I've retested this: ✅ Global styles can be applied to all current blocks opting into I'll get some fresh eyes on this PR and look to merge it later this afternoon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have retested after your latest changes @aaronrobertshaw and the following still works for me:
✅ Global styles can be applied to all current blocks opting into __experimentalDuotone
✅ Global styles are applied correctly for the Image block that uses supports.filter.duotone
✅ Individual block styles in the editor can be applied for blocks using __experimentalDuotone
✅ Individual block styles work for the Image block using supports.filter.duotone
✅ Duotone filters via global styles apply for all blocks on frontend
✅ Individual block duotone filters override global duotone filters on the frontend
I just cherry-picked this PR to the release/15.5 branch to get it included in the next release: fa6e2a6 |
Related:
What?
This PR fixes global-level duotone (see #49393 (comment) for details) and make duotone work like any other selector.
Why?
How?
Make duotone selectors work like any other selector:
Updates the selectors API functions to scope the old
supports.color.__experimentalDuotone
selector automatically.Stabilize duotone support flag under
filter.duotone
block_type_metadata_settings
filter to migrate oldcolor.__experimentalDuotone
flag.Test
selectors.filter.duotone
) and two cover blocks (supports.color.__experimentalDuotone
).purple-green
duotone for one of the images and one of the covers.The expected result is that the blocks with block-level duotone have it applied and the other ones use the global-level duotone in any context (front-end, post editor, site editor).
No other
img
elements of the page have any duotone applied (check the user avatar at the top-right, for example).Front-end:
Post editor: